roborev: Combined Review (c97a966)

The PR has four medium-severity correctness and portability issues in IMAP folder selection, error handling, and catalog counts.

Medium

• internal/imap/client.go:547 — Gmail optimization bypasses folder filters. Gmail detection replaces the filtered mailbox list with All Mail, Trash, and Junk. As a result, --folders=INBOX can sync the entire account and excluded Trash/Junk folders are reintroduced.

  • Fix: Derive the Gmail optimization from the effective filtered mailbox set, or disable it when folder filters are active and enumerate only selected folders with deduplication.

• internal/imap/client.go:531 — CLI folder selection has incorrect precedence and exclusions. CLI includes are applied after configured folders have already narrowed the list, so they cannot override configuration. Additionally, --skip-folders is ignored whenever --folders is non-empty.

  • Fix: Select one effective include list—CLI includes when provided, otherwise configured folders—then call filterMailboxes once against the original LIST results with both the effective includes and CLI exclusions.

• internal/store/messages.go:1517 — Catalog counts use the canonical source mailbox instead of actual folder membership. Providers using All Mail as the canonical mailbox can incorrectly report zero cataloged messages for Inbox and other labeled folders. The query also relies on SQLite-specific INSTR, preventing counts on PostgreSQL.

  • Fix: Use portable SQL to count live messages by joining message_labels and labels, scoped to the source.

• internal/imap/client.go:762 — IMAP LIST errors are silently reported as empty success. A failed LIST command becomes a nil mailbox slice, causing ListMailboxes and list-folders to return successful empty results instead of the server error.

  • Fix: Return the LIST error from the helper and propagate it through ListMailboxes.

Reviewers: 2 done | Synthesis: codex, 14s | Total: 6m32s

roborev: Combined Review (20b05cc)

The PR needs a medium-severity fix: Gmail folder filters can be bypassed by the \All optimization.

Medium

• internal/imap/client.go:545 — Gmail’s \All optimization replaces the filtered mailbox list with All Mail, Trash, and Junk. Because these special mailboxes are discovered before filtering, --folders and --skip-folders are effectively ignored for Gmail IMAP accounts.

  Suggested fix: Disable the optimization when filters are active, or intersect the special mailboxes with the filtered list. Add coverage with [Gmail]/All Mail configured as the \All mailbox.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 5m46s

please excuse the force-pushing. i messed up the history and decided to just squash everything.

i'm not using gmail (or postgres, oops), so how would you like me to resolve the \All feedback?

roborev: Combined Review (29d1801)

Medium-severity issue found: Gmail’s All Mail optimization can bypass configured folder filters.

Medium

• Folder filtering bypassed for Gmail IMAP servers — internal/imap/client.go:540
  After allMailboxes is filtered, the Gmail \All optimization replaces listMailboxes with unfiltered All Mail, Trash, and Junk mailboxes. Excluded folders may therefore still be synced, and an include filter such as INBOX can import nearly the entire account.

  Fix: Disable or constrain the Gmail All Mail optimization whenever folder filters are active, ensuring canonical enumeration remains limited to the filtered mailbox set. Add a test with allMailFolder set to [Gmail]/All Mail.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 4m26s

roborev: Combined Review (29d1801)

The IMAP folder-filtering changes have two medium-severity correctness and interface issues that should be addressed before merging.

Medium

• Gmail canonical mailbox selection bypasses folder filters — internal/imap/client.go:551
  Gmail-style servers replace the filtered mailbox list with All Mail, Trash, and Junk. Consequently, --folders INBOX may sync every message, while --skip-folders Trash may still enumerate Trash. Preserve the effective filter when selecting canonical Gmail mailboxes, and add coverage for a server advertising \All, \Trash, and \Junk.

• Folder flags are unavailable on the normal sync command — cmd/msgvault/cmd/syncfull.go:829
  IMAP accounts invoked through sync use the same folder-based full-sync path, and list-folders help directs users to these flags on sync, yet msgvault sync --folders ... is rejected. Register and propagate the folder options through sync and its HTTP endpoint, or explicitly limit and document the feature as sync-full only.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 2m39s

@mattf force pushing is very normal in this agent era. rebases are now basically free. huzzah!

taking a look

roborev: Combined Review (4060875)

The changes are generally sound, but mailbox names containing commas can be misparsed during API sync.

Medium

• cmd/msgvault/cmd/serve.go:895 — Repeatable API folder values are collapsed into a comma-separated flag and later split on commas. A valid mailbox name containing a comma is silently treated as multiple folders, potentially syncing none of that mailbox’s messages.
  • Fix: Preserve values with repeatable string-array CLI flags end-to-end, or use an escaping format that supports arbitrary mailbox names.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 7m18s

roborev: Combined Review (63f320b)

Code changes need revision due to one medium-severity folder-handling issue.

Medium

• Comma-containing IMAP folder names are corrupted — cmd/msgvault/cmd/serve.go:969, cmd/msgvault/cmd/syncfull.go:553
  Folder arrays are joined and re-split using commas, so valid mailbox names containing commas may select or exclude unintended folders. Preserve folder names as distinct repeatable arguments, such as Cobra string-array flags, instead of CSV encoding.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 8m56s

i went with the repeated arg approach after trying an escaping approach w/ \,. i think the escaping was going to be too complicated for users as they'd contend with both shell & msgvault escaping.

granted, a , in a folder name is likely a rare event.

roborev: Combined Review (28513e1)

Medium-severity correctness issue found in filtered IMAP rescans; no security issues identified.

Medium

• internal/imap/client.go:540 — Filtering allMailboxes before label-map construction and cross-mailbox deduplication makes a partial folder view appear authoritative. On accounts with an \All mailbox, a filtered rescan excluding All Mail can rewrite an existing message’s stable source_message_id to a selected folder’s UID and replace previously known labels with only the filtered labels.

  Recommended fix: When folder filtering is active, preserve canonical IDs and merge existing labels, or retain an unfiltered mailbox view for deduplication metadata. Add a regression test covering a filtered rescan of an already-synced message.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 7m46s

roborev: Combined Review (88a2932)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 12m13s

roborev: Combined Review (6abf45e)

Code is generally solid, but one medium-severity terminal injection issue should be fixed.

Medium

• Terminal escape injection through IMAP mailbox names — cmd/msgvault/cmd/listfolders.go:143

  The list-folders command prints server-provided mailbox names directly to the terminal. A malicious mailbox name containing ANSI/OSC control sequences could forge output, alter terminal state, or manipulate clipboard contents. This also remains unsanitized when streamed through the daemon.

  Sanitize f.Mailbox with the existing textutil.SanitizeTerminal helper before formatting it.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 8m46s

roborev: Combined Review (821990a)

Changes need a fix for partial-snapshot label merging.

Medium

• internal/sync/sync.go:764 — Partial-snapshot label merging occurs only during RFC822 deduplication. Messages already archived under the same composite ID are filtered out by processBatch before reaching this branch, so a filtered Gmail-style IMAP rescan through All Mail cannot add newly discovered folder labels.
  • Fix: Merge labels for exact-ID matches when snapshots are incomplete. Add a regression test where a filtered rescan returns an existing composite ID with an additional label.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 9m5s

I can take over from here

roborev: Combined Review (841e9d8)

The changes introduce two medium-severity sync correctness and efficiency issues.

Medium

• internal/sync/sync.go:208 — Complete snapshots acknowledge existing exact IDs without refreshing their labels. Labels merged during a filtered scan can remain permanently stale after remote membership changes, even after an unfiltered forced rescan, if the canonical IMAP ID remains stable. For complete snapshots, replace exact-ID label associations using authoritative label metadata and add a regression test covering filtered label addition, remote removal, and an unfiltered rescan.

• internal/sync/sync.go:212 — Existing messages in incomplete snapshots are added to fetchIDs, and the IMAP batch fetch requests BODY.PEEK[]. On servers with an \All mailbox, a folder change can redownload every selected message—including attachments—only to merge labels. Add a label-only metadata path for existing IDs and reserve raw MIME fetching for new messages.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 7m16s

roborev: Combined Review (7504da5)

One-line verdict: Two medium-severity IMAP synchronization issues could cause label loss or unnecessarily abort batch processing.

Medium

• Incomplete label snapshots can remove valid labels — internal/imap/client.go:134
LabelsSnapshotComplete considers only configured filters and ignores labelMapComplete. If mailbox membership collection fails for any mailbox, the snapshot is still treated as authoritative, so ReplaceMessageLabels may remove labels missing only because of that failure. Track label-map/enumeration completeness on the client and return false when collection is incomplete, causing labels to be merged instead of replaced.

• A vanished UID aborts the entire metadata batch — internal/imap/batch_fetch.go:405
  If an existing UID is expunged between listing and metadata fetching, the resulting error aborts the whole label batch and marks every message in it as failed. Preserve per-message missing/fetch errors, as the raw-message batch path does, so the syncer can skip or individually record vanished messages while continuing with the remaining results.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 5m5s

I can take over from here

👍

roborev: Combined Review (cec0d51)

Medium-severity label snapshot completeness bugs could cause existing folder memberships to be removed during IMAP rescans.

Medium

• internal/imap/client.go:614 — Servers that do not advertise a \All mailbox never build msgIDToLabels, yet the snapshot is marked complete. Consequently, GetMessageLabelsBatch returns only the current mailbox, and ReplaceMessageLabels may discard other folder memberships previously accumulated for the same message.

  • Fix: Build the membership map for these servers, or mark their snapshots incomplete and merge labels instead of replacing them.

• internal/imap/client.go:139 — LabelsSnapshotComplete ignores active --after and --before filters even though those filters constrain membership collection. A date-bounded rescan can therefore treat a partial label view as authoritative and remove labels from existing messages.

  • Fix: Require since and before to be zero before declaring the snapshot complete, and add a regression test for a date-filtered rescan.

Reviewers: 2 done | Synthesis: codex, 12s | Total: 8m9s

roborev: Combined Review (51b9231)

One medium-severity correctness issue was found; no security vulnerabilities were identified.

Medium

• internal/imap/batch_fetch.go:365 — Label-only fetches bypass cross-mailbox deduplication. The label-only path does not seed seenRFC822IDs. On non-Gmail servers where \All overlaps another mailbox, an existing All Mail|UID is refreshed, then fetched again under an alternate composite ID. The duplicate can overwrite the canonical source ID and replace complete labels with only the alternate mailbox, causing IDs and labels to oscillate across syncs.

  Suggested fix: Record successful \All label fetches in seenRFC822IDs (or otherwise preserve deduplication ordering across label-only and raw fetches), and add a rescan test where one RFC822 message appears in both \All and another mailbox.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 5m43s

roborev: Combined Review (64be577)

The change has one medium-severity correctness issue; no security vulnerabilities were identified.

Medium

• Incomplete reconciliation on servers without \All — internal/imap/client.go:620, internal/sync/sync.go:861

  An unfiltered sync-full --noresume is always treated as incomplete when the server lacks a \All mailbox. If a message moves from INBOX|42 to Trash|99, deduplication adds Trash but does not remove INBOX or update the composite ID. The stored ID may reference a vanished UID, breaking later IMAP trash/delete operations.

  Suggested fix: Treat fully enumerated, unfiltered scans as authoritative by building a complete Message-ID membership map before ingestion or reconciling memberships and composite IDs afterward. Keep filtered and high-water incremental scans additive.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 9m20s

roborev: Combined Review (e89b405)

Code review found two medium-severity sync reconciliation issues; no security issues were identified.

Medium

• Stale labels and source IDs in cached views — internal/sync/sync.go:262, internal/sync/sync.go:870
  Successful label and canonical-ID reconciliation does not increment checkpoint.MessagesUpdated. Because cache invalidation relies on this counter, rescans that only modify existing messages can leave Parquet-backed TUI and web views stale indefinitely. Track reconciliation changes in batchResult, increment MessagesUpdated, and persist the count so affected messages are rewritten during cache rebuilding.

• Invalid composite IDs retained after mailbox moves — internal/sync/sync.go:864
  Every incomplete snapshot preserves the old composite ID, including normal high-water scans. When a message moves between mailboxes, the old mailbox|uid may no longer exist; sync adds the new label but retains the vanished UID, causing later fetch or deletion operations to target the wrong remote location. Distinguish explicitly filtered snapshots from high-water scans, and update or validate the canonical composite ID when the stored copy no longer exists while continuing to merge labels conservatively.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 8m35s

roborev: Combined Review (58dc188)

One medium-severity correctness issue remains.

Medium

• Cross-mailbox deduplication can permanently omit a mailbox label — internal/imap/client.go:644
  When buildLabelMap returns an incomplete membership map, cross-mailbox deduplication remains active. If a mailbox’s header fetch fails but its later raw fetch succeeds, an overlapping message can become an acknowledged dedup stub before its mailbox label is recorded. This may advance the folder high-water mark and prevent the missing label from being retried.

  Suggested fix: Enable seenRFC822IDs only when the membership map is complete, or ensure copies deduplicated under an incomplete map reconcile their current mailbox label without prematurely advancing folder state.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 12m22s

roborev: Combined Review (e895b8f)

Medium-severity reconciliation bug found; no Critical or High issues identified.

Medium

• internal/imap/batch_fetch.go:516 — SourceMessageExists treats a missing or renamed mailbox as an inconclusive SELECT error. During high-water reconciliation, this permanently prevents replacement of the stale canonical ID, leaves the new mailbox copy unacknowledged, and causes every subsequent sync to retry the same failure.
  • Fix: If the mailbox is absent from the current unfiltered LIST cache, return (false, nil). Preserve retryable errors only when a currently listed mailbox cannot be selected.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 14m1s

roborev: Combined Review (ebca96a)

Code review found two medium-severity reconciliation risks; no material security regressions were identified.

Medium

• internal/sync/sync.go:906 — A filtered scan preserves the old composite ID for a deduplicated message while acknowledging the newly observed ID and advancing that folder’s high-water state. After a move such as INBOX|1 → Trash|1, a later unfiltered scan may skip both unchanged folders, leaving the source ID permanently stale.

  • Fix: Do not advance the filtered folder’s state for unresolved alternate IDs, or persist a reconciliation marker that forces validation during the next unfiltered scan.

• internal/imap/batch_fetch.go:506 — SourceMessageExists treats any successful fetch for mailbox|uid as proof that the original message still exists. Following a UIDVALIDITY change, that UID may identify an unrelated message, preserving a stale ID and potentially directing later IMAP operations at the wrong message.

  • Fix: Treat a UIDVALIDITY mismatch as definitive invalidation and/or compare the fetched RFC822 Message-ID with the expected stored Message-ID.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 10m29s

roborev: Combined Review (76edc65)

The changes are generally sound, but one medium-severity sync correctness issue remains.

Medium

• internal/sync/sync.go:222 — Existing composite IDs are trusted without validating the fetched RFC822MessageID. After an IMAP UIDVALIDITY change, a reused mailbox|uid may refer to an unrelated message; the message is classified as existing, receives only a label refresh, and is never archived.
  • Suggested fix: Compare exact-ID metadata with the stored RFC822 Message-ID, particularly across invalidated UID epochs. Preserve or rekey the previously archived row before ingesting the newly assigned message.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 10m5s

roborev: Combined Review (cab7a6b)

High-severity correctness issue: UID reuse can suppress message archival; statistics can also report negative skipped counts.

High

• internal/imap/batch_fetch.go:375 — Label-only fetches seed seenRFC822IDs before validating stored and fetched identities. If a UID is reused or UIDVALIDITY changes, the archived row is invalidated and a raw refetch is requested, but that refetch becomes a dedup stub because its Message-ID was already seeded. The replacement message remains unarchived even though folder state may advance, causing later syncs to skip it.
  • Fix: Seed dedup state only after identity validation succeeds, or make identity-mismatch refetches remove/bypass the premature dedup entry. Add coverage for UID reuse during a complete/--noresume scan.

Medium

• internal/sync/sync.go:345 — UID reuse counts one listed item as both updated when the old row is invalidated and added when its replacement is archived. Consequently, MessagesSkipped = processed - added - updated can produce -1 for a one-message replacement and persist inconsistent statistics.
  • Fix: Count replacements in one mutually exclusive category, or track skipped items directly instead of deriving them from overlapping mutation counts.

Reviewers: 2 done | Synthesis: codex, 12s | Total: 6m50s

roborev: Combined Review (69e1ded)

Medium-severity identity validation flaw found; no High or Critical issues.

Medium

• internal/imap/batch_fetch.go:591 — Exact-ID validation treats two missing RFC822 Message-ID values as an identity match. With --noresume, saved folder states are omitted, so a recreated mailbox can reuse a UID for a different message lacking a Message-ID; the replacement may be acknowledged without fetching its raw MIME and never archived.
  • Fix: Retain prior UIDVALIDITY metadata when forcing enumeration. Treat missing identities as inconclusive unless another stable check, such as raw-content comparison, confirms the match. Add coverage for UID reuse when both messages omit Message-ID.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 10m47s

roborev: Combined Review (8fa4402)

The changes are generally sound with no security regressions identified, but two medium-severity correctness issues should be addressed.

Medium

• internal/imap/batch_fetch.go:607 — Missing Message-ID can trigger repeated false UID-reuse detection. A Message-ID present on only one side is treated as conclusive evidence of UID reuse. Archived messages may lack this metadata after MIME parsing falls back to a placeholder, while the header-only parser still extracts it. Each rescan can then invalidate the live source ID and archive the same raw message again.

  • Fix: Treat either missing identity as inconclusive unless UIDVALIDITY or absence proves replacement. Fall back to the existing raw-MIME comparison and optionally backfill the Message-ID.

• internal/sync/sync.go:1077 — Complete-snapshot dedup can unnecessarily replace a valid canonical source ID. If an overlapping mailbox copy appears on an earlier 100-message page than the archived exact ID, the exact ID has not yet seeded dedup, so the existing composite source ID is rekeyed even though it remains valid.

  • Fix: Verify whether the existing source ID still resolves to the same message before rekeying, including for complete snapshots. Preserve it and merge labels when valid, and add a cross-page overlap test.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 12m1s

roborev: Combined Review (8c60e35)

The change is generally sound, but filtered syncs can incorrectly access excluded mailboxes during deduplication.

Medium

• internal/sync/sync.go:1088 — Filtered syncs still validate deduplicated messages by fetching their previously stored mailbox, even when that mailbox is excluded. This can access deliberately skipped folders, fail messages when excluded mailboxes are inaccessible, and add an unnecessary FETCH per overlapping message.
  • Fix: Make source validation aware of the effective folder selection. When the stored mailbox is outside the current snapshot, preserve the canonical ID and merge labels, deferring validation until that mailbox is included.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 13m6s

roborev: Combined Review (5a955f2)

Code review found two medium-severity IMAP synchronization issues; no security regressions were identified.

Medium

• internal/sync/sync.go:556 — Deferred messages can be permanently skipped

  Deferred IMAP identities remain unacknowledged but do not increase the error count. runFullSync then saves every observed folder state, potentially advancing UIDNEXT beyond the deferred message. A later unfiltered sync may skip both folders, preventing identity reconciliation.

  Fix: Exclude folders containing pending unacknowledged messages from ObservedFolderStates, or otherwise prevent their states from being persisted during the final bulk save.

• internal/sync/sync.go:1125 — Canonical \All source IDs are never adopted

  Reconciliation preserves an existing non-\All source ID whenever it still resolves. If an account is first synced with --folders INBOX and later without filters, the canonical \All identity is not adopted. Subsequent unfiltered syncs may therefore miss the exact-ID fast path and repeatedly download raw MIME when the account changes.

  Fix: During a complete snapshot, allow the IMAP client to identify the preferred canonical source ID and rekey valid non-\All IDs when their \All counterpart is observed.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 16m41s

roborev: Combined Review (9ad43f5)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 11m11s

Will merge pending CI. If I broke something in all my grinding please open another PR!

@wesm that was a pretty epic back and forth

roborev: Combined Review (704fffa)

No Medium, High, or Critical findings; the reviewed changes are clean.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 16m59s